Add opentelemetry by lizard-boy · Pull Request #67 · grepdemos/server

lizard-boy · 2024-10-19T09:05:35Z

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Adds opentelemetry-based tracing to server projects. Note: tracing is used exclusively as a system profiler to aid in performance optimizations, not intended to trace usage at all. Please let me know if you find some leaked PII in here!

Code changes

launch.json/tasks.json/launchSettings.json Include Events project in builds, correct the port for events processor self host to be consistent with visual studio build settings and pattern for self-host version of other project (it was previously using the Events port by mistake).
docker-compose: include jaeger as a tool to analyze tracing
api/startup: Add tracing attribute to controller action execution. Turn on middleware telemetry to monitor execution of middlewares
tracingAttribute: Logs tracing even at begin and end of action execution as well as wrapping result processing
entityFramework.csproj: include EF-specific opentelemetry package
DiagnosticsConfig: helper class to configure activity and service information in shared service configuration helpers.
MiddlewareAnalysisDiagnosticsHelper: The idea behind this is to wrap all middleware with trace activities to track their execution time -- much like the controller action wrapping done by the TracingAttribute. It works OK, but the library it relies on, Microsoft.Extensions.MiddlewareAnalysis, is poorly documented and seems to be long-forgotten. Sometimes it seems to randomly miss middlewares, but It certainly catches more than if we didn't have it...
diagnosticsHelpers: static class to add configuration helpers for diagnostics purposes
serviceCollectionExtension: Add openetelemetry and diagnostic event emissions listened to by MiddlewareAnalysisDiagnosticsHelper

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

Greptile Summary

This pull request introduces OpenTelemetry-based tracing to the server projects, focusing on system profiling for performance optimization without tracking user-specific data.

Added Jaeger service in docker-compose.yml for distributed tracing analysis
Implemented TracingAttribute in src/Core/Utilities/TracingAttribute.cs to wrap controller actions and results with tracing activities
Created DiagnosticsConfig in src/SharedWeb/Health/DiagnosticsConfig.cs to configure ActivitySource based on project name
Introduced MiddlewareAnalysisDiagnosticAdapter in src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs for middleware execution tracing
Modified src/Api/Startup.cs to integrate tracing and middleware telemetry in the application pipeline

Use Jaeger for tracing

Add diagnostics for aspdotnet core, middleware, sqlserver, ef core, http clients, and action/result filters for controller actions

greptile-apps

_{11 file(s) reviewed, 14 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-19T09:06:15Z

src/Api/Startup.cs

        {
            config.Conventions.Add(new ApiExplorerGroupConvention());
            config.Conventions.Add(new PublicApiControllersModelConvention());
+            config.Filters.Add(new TracingAttribute()); // Add tracing to controller actions and results


style: Adding tracing to all controller actions may introduce performance overhead. Consider selective tracing for critical paths only.

greptile-apps · 2024-10-19T09:06:15Z

src/Api/Startup.cs

        app.UseSerilog(env, appLifetime, globalSettings);

+        // Middleware telemetry
+        DiagnosticsHelpers.AddMiddlewareDiagnostics(serviceProvider);


logic: Middleware diagnostics may miss some middleware. Ensure comprehensive coverage or implement fallback monitoring.

greptile-apps · 2024-10-19T09:06:16Z

src/Api/Startup.cs

+        ILogger<Startup> logger,
+        IServiceProvider serviceProvider)
    {
        IdentityModelEventSource.ShowPII = true;


logic: Showing PII in logs could lead to security vulnerabilities. Ensure this is disabled in production environments.

greptile-apps · 2024-10-19T09:06:42Z

src/Core/Core.csproj

+    <PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" Version="1.5.1-beta.1" />
+    <PackageReference Include="OpenTelemetry.Instrumentation.Http" Version="1.5.1-beta.1" />
+    <PackageReference Include="OpenTelemetry.Instrumentation.SqlClient" Version="1.5.1-beta.1" />


style: These packages are in beta. Consider using stable versions for production, or document the use of beta packages.

greptile-apps · 2024-10-19T09:07:01Z

src/Core/Utilities/TracingAttribute.cs

+            _actionActivity.AddTag("exception", context.Exception.ToString());
+            _actionActivity.AddTag("exceptionStackTrace", context.Exception.StackTrace);


style: Be cautious about logging full exception details, as it may expose sensitive information. Consider logging only exception type and message

greptile-apps · 2024-10-19T09:08:17Z

src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs

+        if (_activitySource.StartActivity(name, ActivityKind.Server) is Activity activity)
+        {
+            _activities[name] = activity;
+        }


logic: This could potentially lead to a memory leak if activities are not properly removed. Consider implementing a cleanup mechanism

greptile-apps · 2024-10-19T09:08:17Z

src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs

+        _activities[name].AddTag("exception", exception.ToString());
+        _activities[name].AddTag("exceptionStackTrace", exception.StackTrace);


style: Logging full exception details might expose sensitive information. Consider logging only necessary information

greptile-apps · 2024-10-19T09:08:18Z

src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs

+        if (name == "Microsoft.AspNetCore.MiddlewareAnalysis.AnalysisMiddleware" || !_activities.ContainsKey(name))
+        {
+            return;
+        }


logic: The activity might have already been stopped if an exception occurred. Add a check to ensure the activity is still running before stopping it

greptile-apps · 2024-10-19T09:08:19Z

src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs

+        if (name == "Microsoft.AspNetCore.MiddlewareAnalysis.AnalysisMiddleware" || !_activities.ContainsKey(name))
+        {
+            return;
+        }
+        _activities[name].Stop();
+    }


style: Consider removing the activity from the dictionary after stopping it to prevent potential memory leaks

greptile-apps · 2024-10-19T09:09:14Z

src/SharedWeb/Utilities/ServiceCollectionExtensions.cs

+        services.AddOpenTelemetry().WithTracing(tracerProviderBuilder =>
+                tracerProviderBuilder
+                    .AddSource(diagnosticsConfig.ActivitySource.Name)
+                    .ConfigureResource(resource => resource
+                        .AddService(diagnosticsConfig.ServiceName))
+                    .AddAspNetCoreInstrumentation()
+                    .AddSqlClientInstrumentation()
+                    .AddEntityFrameworkCoreInstrumentation()
+                    .AddHttpClientInstrumentation()
+                    .AddOtlpExporter());


style: Consider adding sampling to reduce the volume of telemetry data in production

MGibson1 added 2 commits June 26, 2024 14:53

Add opentelemetry to shared web

01ce8d7

Use Jaeger for tracing

Add open telemetry

d1a83cf

Add diagnostics for aspdotnet core, middleware, sqlserver, ef core, http clients, and action/result filters for controller actions

greptile-apps bot reviewed Oct 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Add opentelemetry#67

Add opentelemetry#67
lizard-boy wants to merge 2 commits intomainfrom
add-opentelemetry

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

greptile-apps bot Oct 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		_actionActivity.AddTag("exception", context.Exception.ToString());
		_actionActivity.AddTag("exceptionStackTrace", context.Exception.StackTrace);

		_activities[name].AddTag("exception", exception.ToString());
		_activities[name].AddTag("exceptionStackTrace", exception.StackTrace);

Comments

Conversation

lizard-boy commented Oct 19, 2024 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Objective

Code changes

Before you submit

Greptile Summary

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading